Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow mixing of integers and pointers in Crystal::System.print_error #14149

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

When an external library calls Crystal::System.print_error with an argument, we do not have control over the argument type; in the case of Boehm GC, that argument is always an unsigned integer, even though the actual format specifier might be %s or %p. This PR makes those argument types interchangeable. (An integer or pointer is interpreted as the address to a null-terminated string for %s, never the address of a Crystal String.)

Also uses %p consistently in our own calls to .print_error.

@straight-shoota
Copy link
Member

Making pointer and integers interchangeable seems like a good idea. That should be safe and sound.

But I'm worried about treating arbitrary integers as string pointers. This is potentially unsafe and can easily crash the process.

I'm not sure if there's a different way to enable the use case with libgc.
Perhaps at least we can restrict %s to work only for the integer type that corresponds to the size of a pointer address? (~ SizeT)

@straight-shoota
Copy link
Member

Also I think the refactoring part should be extracted as a separate PR. It's indendent of the designator type related changes.

@HertzDevil
Copy link
Contributor Author

I don't think restricting the integer type makes the function any safer, because that implies a C library has to indicate the impossibility of pointers by using a 64-bit type on 32-bit systems but a 32-bit type on 64-bit systems.

@straight-shoota
Copy link
Member

straight-shoota commented Jan 4, 2024

IMO indicating the impossibility of a pointer is not that relevant. If a lib wants to ensure proper usage, they should prevent improper usage of the designator.
My goal would be to make the margin of error as tight as possible. For example, an 8-bit integer type should never be expected to be an address. Or a signed integer type.

@HertzDevil
Copy link
Contributor Author

That only affects our own calls to .print_error, which are not the focus of this PR. LibGC::Word is going to be the same whether we allow all integer types or just unsigned pointer-sized ones.

@straight-shoota
Copy link
Member

straight-shoota commented Jan 4, 2024

Calls from Crystal land might not be the focus of this PR, but it's a use case of this method. And this change indiscriminately enables %s for Int::Primitive. So I think it affects our own calls as well. Unless I'm missing something here?

@HertzDevil
Copy link
Contributor Author

I honestly don't see a problem about that.

@straight-shoota
Copy link
Member

straight-shoota commented Jan 4, 2024

My concern is about the following program:

Crystal::System.print_error "%s", 1_i8

On master, it prints (???) which I think is correct and expected because an Int8 cannot be a valid address in any realistic scenario (except for zero/null value maybe).
With this patch, it triggers an invalid memory access. That would be avoidable if the type filter was more restrictive than Int::Primitive.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jan 4, 2024

Exactly. I don't see a problem with allowing Int::Primitive here. Otherwise the caller would simply add to_u64 at the call site and that doesn't make the .print_error call any safer.

@straight-shoota
Copy link
Member

Yeah, but then it would be explicit. 🤷 Of course we cannot prevent accidental submission of random UInt64 numbers. But we can prevent anything that's not a UInt64 from being erroneously interpreted as a pointer.

@HertzDevil
Copy link
Contributor Author

You have to include UInt32 since native pointers have that size on 32-bit targets. The most I'd allow is Int32 | UInt32 | Int64 | UInt64 on all platforms, since there is no guarantee C libraries would give us unsigned values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants